Skip to content

feat(valgrind): add --experimental-rep-bulk-cost flag - #620

Open
not-matthias wants to merge 1 commit into
mainfrom
feat-experimental-rep-bulk-cost
Open

not-matthias wants to merge 1 commit into
mainfrom
feat-experimental-rep-bulk-cost

Conversation

@not-matthias

@not-matthias not-matthias commented Nov 10, 2026 •

Copy link
Copy Markdown
Member

Adds an opt-in --experimental-rep-bulk-cost flag (env CODSPEED_EXPERIMENTAL_REP_BULK_COST). When set together with --cycle-estimation, the runner passes --rep-bulk-cost=yes to Callgrind, which prices amd64 rep movs/rep stos from a calibrated bulk cost curve instead of the per-element model.

The option is off by default in valgrind-codspeed, so default results are unchanged. Once we are happy with the results, the runner can turn it on by default.

Depends on CodSpeedHQ/valgrind-codspeed#60. Requires a valgrind-codspeed release containing it and a bump of VALGRIND_CODSPEED_ITERATION before merging: older valgrind builds reject the unknown option.

Closes COD-4199

@not-matthias
not-matthias force-pushed the feat-experimental-rep-bulk-cost branch from 235d1d4 to f4a85b0 Compare November 10, 2026 14:19
@codspeed

codspeed Bot commented Nov 10, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

2 warnings

⚠️ Walltime results may be unreliable

Some benchmarks used the Walltime instrument outside CodSpeed Macro Runners, which can make the results inconsistent.

Learn more about Macro Runners

See affected benchmarks

⚠️ 8 benchmarks are dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

✅ 53 untouched benchmarks
⏩ 6 skipped benchmarks1


Comparing feat-experimental-rep-bulk-cost (f4a85b0) with main (34b48f1)

Open in CodSpeed

Footnotes

  1. 6 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@not-matthias
not-matthias marked this pull request as ready for review November 10, 2026 15:19
@greptile-apps

greptile-apps Bot commented Nov 10, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 4/5

[Medium impact] The change reaches benchmark runs that opt into the new Callgrind cost model.

The code changes appear safe to merge once the already acknowledged Valgrind release prerequisite is satisfied; the test suggestion is non-blocking.

Fix All in Claude CodeFindings

  1. P2 Cost model conditions lack tests ▶
Fix with agent prompt
### Issue 1
src/executor/valgrind/measure.rs:50-52
The new `get_valgrind_args` branch has no tests; the existing tests only cover the shell wrapper. Add checks that `--rep-bulk-cost=yes` appears only for Callgrind when both settings are true, and stays absent by default, with cycle estimation disabled, and with Tracegrind. This would catch future changes that silently alter benchmark costs.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

Adds opt-in --experimental-rep-bulk-cost and CODSPEED_EXPERIMENTAL_REP_BULK_COST support.

  • Benchmark runs can opt into Callgrind's calibrated REP-copy cost model.

Known prerequisite acknowledged by not-matthias: publish a compatible Valgrind release and bump VALGRIND_CODSPEED_ITERATION before merging, because older builds reject the option.

Reviews (1) · Last reviewed commit: "feat(valgrind): add --experimental-rep-b..." · Reviewed by Greptile

Comment thread src/executor/valgrind/measure.rs Resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant